feat: added tests for socket apis by hritikgupta · Pull Request #1900 · bytecodealliance/wasm-micro-runtime

hritikgupta · 2023-01-19T00:42:12Z

No description provided.

lum1n0us · 2023-01-27T03:32:49Z

We'd better not check in a binary file (core/iwasm/libraries/lib-socket/test/tcp)

lum1n0us · 2023-01-27T03:41:43Z

@loganek @wenyongh we usually keep similar code in samples/socket-api, is it better if we also store above code there?

wenyongh · 2023-01-27T10:37:34Z

We'd better not check in a binary file (core/iwasm/libraries/lib-socket/test/tcp)

Yes, don't check in wasm binary file here.

wenyongh · 2023-01-27T10:44:10Z

@loganek @wenyongh we usually keep similar code in samples/socket-api, is it better if we also store above code there?

No, had better move the source files into samples/socket-api/src, rename the sample files with suitable names, and update document about how to run the samples and what the output should be.

@hritikgupta Could you add the files as samples in samples/socket-api/src and update the document and build scripts? Thanks.

loganek · 2023-01-27T10:52:17Z

@wenyongh , @lum1n0us so the intention of this is to run those tests as part of WASI test suite. I don't have strong opinion about where to put those tests, but my preference would be to keep examples and tests separate (even though there's some overlap).

loganek · 2023-01-27T10:53:17Z

@hritikgupta could you also add integration with wasi testsuite to this PR? I think the change might be a bit confusing without that.

wenyongh · 2023-01-27T11:14:19Z

@wenyongh , @lum1n0us so the intention of this is to run those tests as part of WASI test suite. I don't have strong opinion about where to put those tests, but my preference would be to keep examples and tests separate (even though there's some overlap).

Got it, if they are only tests, it should be good to put them here, and yes, had better add them to CI test.

hritikgupta · 2023-01-27T11:33:36Z

We'd better not check in a binary file (core/iwasm/libraries/lib-socket/test/tcp)

Yes, don't check in wasm binary file here.

Right right, this got committed accidentally while I was testing the script, will remove this as a part of the commit addressing the remaining review comments.

hritikgupta · 2023-03-08T20:05:04Z

@wenyongh / @loganek — addressed the changes, does this look good to merge?

wenyongh · 2023-03-09T01:10:53Z

          ]
+        wasi_sdk_release:
+          [
+            "https://github.com/WebAssembly/wasi-sdk/releases/download/wasi-sdk-19/wasi-sdk-19.0-linux.tar.gz",


The same change was already added in #1963 so now no need to add this line here after it was merged.
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/.github/workflows/compilation_on_android_ubuntu.yml#L462

wenyongh · 2023-03-09T01:12:36Z

+          cd /opt
+          sudo wget ${{ matrix.wasi_sdk_release }}
+          sudo tar -xzf wasi-sdk-19.0-*.tar.gz
+          sudo mv wasi-sdk-19.0 wasi-sdk


Same as above, see
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/.github/workflows/compilation_on_android_ubuntu.yml#L500-L523

wenyongh · 2023-03-09T01:13:06Z

+            NM=/opt/wasi-sdk/bin/llvm-nm \
+            CC=/opt/wasi-sdk/bin/clang \
+            THREAD_MODEL=posix
+        working-directory: core/deps


Same as above

wenyongh · 2023-03-09T01:16:27Z

@wenyongh / @loganek — addressed the changes, does this look good to merge?

It is good to me, but need to remove the duplicated changes with #1963.

wenyongh

LGTM

Add internal tests for socket apis (bytecodealliance#1900)